This isn't actually used anywhere. I've left it in for now (since it's already implemented), but we could remove it?

I am usually not very comfortable with boolean flag params, due to their lack of expressivity.

But am I right to say the shouldUpdate may be the default? I only see the flag is kept to false only in tests.

In any case, I would suggest for adding more expressivity the following, if that makes sense to you too:

• make this function private;
• introducing 2 new functions named updateOrAttachFile (where you call the private function with shouldUpdate = true) and attachFileIfNew (with shouldUpdate = false);
• maybe (or maybe not, that's arguable if this is now private) change shouldUpdate: boolean into { shouldUpdate = false }: {shouldUpdate: boolean } = {} or similar;

Interesting thoughts here! I thought a lot about how to split this - I don't really like the flag either, but ended up in the "tangled implementation" case of that article you linked! 🙂

I'd be happy to do it the way you suggest - make the flagged method private, and expose it as two different public methods. It absolutely feels easier to read.

I split this up fully, and separated out the implementation in a way that I think it makes it easier to read. 🙂

Let me know if that works for you!

The comment that used to be line 793 gave some reason that is still useful, I think. So I would add some more context:

I'm not sure if this is still relevant after the other changes made to this function? :)

The code is acceptable as it is for me, so please skip if you don't agree much about my idea.

I feel like, even it introduces a supplementary SQL query to maintain, we would increase the readability by separating the query to check the existence of the file and do the INSERT/UPDATE. Am I right to say the below code would be equivalent?

const exists = await db.run('SELECT 1 from _gristsys_Files where ident = ?', fileIdent); // Not very sure what is returned here
if (exists && shouldUpdate) {
  await db.run('UPDATE _gristsys_Files SET data=?, storageId=? WHERE ident=?', fileData, storageId, fileIdent);
} else {
  await db.run('INSERT INTO _gristsys_Files(ident, data, storageId) VALUES (?, ?, ?)', fileIdent, fileData, storageId);
}
return exists;

These look equivalent to me. I'm not sure why it was originally done by catching the unique constraint, but it looks like the only place in the code where that's done.

I'll make this change, and we'll see if @paulfitz or anyone has any opinion :)

Edit: Change is pushed

Thanks! If there exist doubts, feel free to rollback

We hit an error in one of our test cases due to this change, full writeup is below.

Long story short: The way we handle transactions means we can hit a race condition in this example, and catching the "UNIQUE" constraint violation is the simplest way of avoiding that.

I'll change this back, and add a comment explaining why we handle it this way.

Full explanation:

The SQLiteDB class' function execTransaction is built to merge all calls into a single transaction, rather than run a transaction per-call (which is how I believed it worked).

When several atttachments are being inserted simultaneously in one API call, and because Node's async behaviour is unpredictable, the individual SQL statements are effectively re-ordered at random due to multiple promises running in parallel. This means that even if the SELECT statement says "No records exist", they might be inserted before the "INSERT" actually happens, resulting in a UNIQUE constraint violation.

If we're adding two files at the same time, the resulting order of statements ended up going something like this:
SELECT 1 as fileExists FROM _gristsys_files WHERE ident = ?
SELECT 1 as fileExists FROM _gristsys_files WHERE ident = ?
INSERT INTO main._gristsys_Files (ident, data, storageId) VALUES (?, ?, ?)
INSERT INTO main._gristsys_Files (ident, data, storageId) VALUES (?, ?, ?) // Unique constraint error thrown here.

If they hypothetically had been separate transactions on separate DB connections, the second INSERT would have failed with a "database is locked" error instead - so that's actually no better here.

Hence, catching the UNIQUE constraint error is the cleanest way to avoid this.

Ah right, thank you for your feedback! It makes sense

The new version is much simpler to read, thanks a lot! 🙏

What would you think of the following to avoid the boolean flag param:

  public getFileInfo(fileIdent: string): Promise<FileInfo | null> {
    return this._getFileInfo(fileIdent, 'ident, storageId');
  }
  public getFileInfoWithData(fileIdent: string): Promise<FileInfo | null> {
    return this._getFileInfo(fileIdent, 'ident, storageId, data');
  }
  // ...
  private _getFileInfo(fileIdent: string, columns: string): Promise<FileInfo | null> {
    return this.get(`SELECT ${columns} FROM _gristsys_Files WHERE ident=?`, fileIdent)
      .... // just like what you did
   }

I think splitting them up is a good idea - it definitely makes it more readable.

I think I'd rather duplicate the original function, than pass columns as a string.

That's because there's no risk of bad data accidentally being loaded into the code that formats the result into a typed object (e.g, a missing column).

Additionally, because it lowers the risk of someone doing something silly (such as passing a user-provided column name) in the future.

This is done, please let me know if the refactored version is clearer :)

This function is a bit hard to read due to all of the conditions entangled. The good news is that it is private and you have a public method to call it, so you can rework it serenely.

I feel like it would be less complex to split this function into two different ones. I made the following, but have not tested:

  public async addFile(
    storeId: AttachmentStoreId | undefined,
    fileExtension: string,
    fileData: Buffer
  ): Promise<AddFileResult> {
    const fileIdent = await this._getFileIdentifier(fileExtension, Readable.from(fileData));
    return storeId ? 
      this._addFileToExternalStore(storeId, fileIdent, fileData) : 
      this._addFileToLocalStore(fileIdent, fileData);
  }


  // ...

  private async _addFileToExternalStore(
    destStoreId: AttachmentStoreId,
    fileIdent: string,
    fileData: Buffer
  ): Promise<AddFileResult> {
    const destStore = await this._getStore(destStoreId);
    if (!destStore) {
      this._log.warn({ fileIdent, storeId: destStoreId }, "tried to fetch attachment from an unavailable store");
      throw new StoreNotAvailableError(destStoreId);
    }
    const fileInfoNoData = await this._docStorage.getFileInfo(fileIdent, false);
    const fileExists = fileInfoNoData !== null;
    if (fileExists) {
      // File is already stored in a different store (e.g because store has changed and no migration has happened
      const isFileInTargetStore = destStoreId === fileInfoNoData.storageId;

      // Only exit early if the file is stored elsewhere of if the file exists in the store, 
      // otherwise we should allow users to fix any missing files
      // by proceeding to the normal upload logic.
      const fileAlreadyExistsInStore = isFileInTargetStore && await destStore.exists(this._getDocPoolId(), fileIdent);

      if (!isFileInTargetStore || fileAlreadyExistsInStore) {
        return {
          fileIdent,
          isNewFile: false,
        };
      }
    }
    // There's a possible race condition if anything changed the record between the initial checks
    // in this method, and the database being updated below - any changes will be overwritten.
    // However, the database will always end up referencing a valid file, and the pool-based file
    // deletion guarantees any files in external storage will be cleaned up eventually.
    await this._storeFileInAttachmentStore(destStore, fileIdent, fileData);

    return {
      fileIdent,
      isNewFile: !fileExists,
    };
  }

  private async _addFileToLocalStore(
    fileIdent: string,
    fileData: Buffer
  ): Promise<AddFileResult> {
    const fileInfoNoData = await this._docStorage.getFileInfo(fileIdent, false);
    const fileExists = fileInfoNoData !== null;
    if (!fileExists) {
      await this._storeFileInLocalStorage(fileIdent, fileData);
    }
    return {
      fileIdent,
      isNewFile: !fileExists,
    };
}

I've split this into two, and there's a few duplicate comments now - but I think that's reasonable, as it's definitely a lot easier to understand the logic now.

Typo:

Not a typo here, but I'll clean up the comment to be more intelligible!

It's trying to say:

There's the potential for a race condition here, if any other code modifies the database record between the the checks at the start of this function, and this line of code. The checks could pass then, but fail if evaluated here (due to record changes).

Rephrased to this:

// A race condition can occur here, if the file's database record is modified between the
// `getFileInfoNoData` call earlier in this function and now.
// Any changes made after that point will be overwritten below.
// However, the database will always end up referencing a valid file, and the pool-based file
// deletion guarantees any files in external storage will be cleaned up eventually.

Most of codebase uses semicolon style for interfaces (I think?) unless there's a reason not to.

Also sometimes small non-exported interfaces or classes are put down below the "meat" of the file, so that they don't distract, and that might be nice here (but this is a very weak suggestion, do what you like).

Thank you for adding this.

Great, thanks for adding this.

This is the zero-attachment case too right? I wonder if the zero-attachment case should be external if a default external store is set. Edge case, don't know if there is scope for confusion, feels unimportant.

I think the way we'd need to do this is to add a new state "NO_FILES", return that and either:

• Render it how we like in the frontend
• Or translate it in ActiveDoc to either "EXTERNAL" or "INTERNAL"

I don't mind doing that (tiny bit of work) - what do you think?

The whole point of the summary is to tell if we need transfer or not. So this needs to be sorted out.

Can you put the finally on same line, as } finally {, like the catch.

We avoid == in this codebase because anytime it is used reviewers have to think too much.

ActiveDoc has a well worked out mechanism to avoid parallel changes to the document (see the Share class). The underlying SQLite database also applies changes sequentially. It feels like something has gone wrong somewhere, there is some parallelism introduced for attachments, that is making your life harder.

This is theoretical for the most part - I think it might be possible if several add requests for the file come in at once, and we don't apply the share class to the attachment API in ActiveDoc? Hard to be sure though without studying the code very carefully to trace all paths to this function.

I could probably remove this without any harm, but I thought it might be worth pointing out just in case in comes at some point in the future. Could also add "theoretical" to the start of the comment.

Not sure what the best practice here is, what do you think?

Hmm the user actions applied in ActiveDoc.addAttachments should happen via the Share class ultimately. I feel that the Promise.all in there is silly. But it does up the odds of tickling race conditions like this that might be hard to spot otherwise, so that's good.

Ok, assuming this was the only race, fine to leave as is.

How about sticking in locationSummary for consistency with /transferAll?

Take a look at the endpoint registered above (line 521), it will be matched first and this handler won't be executed.

Ah, nicely spotted, I made the mistake of assuming it was done by specificity. Thanks 🙂

This type should be in common (UserAPI), it is duplicated there.

Also there we have this type:

export interface AttachmentLocationSummary {
  summary: 'INTERNAL'|'EXTERNAL'|'MIXED';
}

The type above looks artificial. In UI and probably elsewhere, the words internal,external,mixed describe the storage of attachments itself. So here I think we don't need to add this summary at the end. Either name it Location, Storage, or add (to the AttachmentLocationSummary) some other bits (like number of files per storage) to make it true summary.

Also in UserAPI there is:

export interface AttachmentTransferStatus {
  status: {
    pendingTransferCount: number;
    isRunning: boolean;
  };
}

This additional property (status) that wraps the content is absolutely not needed.

I'll clean up "DocAttachmentsLocationSummary" - rename it to DocAttachmentsLocation and move it to UserAPI.ts.

I'm not sure where you're seeing export interface AttachmentTransferStatus, that's not showing up in any my searches. Is it something you might have added in the UI code?

This is done now!

Take a look at StringUnion.ts in common. I think we prefere it over enums.

We have something called docChatter (look at CommTypes.ts). This is a way to inform the clients about some backend status in more deterministic ways (so that it works across multiple tabs/clients etc). Before UI lands this AttachmentFileManager needs to leverage it, and inform about the background job status through this channel.

If you want, I can take this task and do it in my PR for the UI.

But this api endpoint still needs to stay.

I think it would be fine to keep the live status part to your PR @berhalak so it lands with the UI. The grist.gouv team would like the functionality in this PR in for the next Grist release if possible.

This interface is never used anywhere, and it conveys a false impression that AttachmentFileManager has >=2 implementations or is injected somehow. Lets remove it.

Done!

The whole point of the summary is to tell if we need transfer or not. So this needs to be sorted out.

Nitpicking: I expect this to work (just to have a strict check and not to rely on startsWith):

return path.parse(fileIdent).name === await checksumFileStream(Readable.from(fileData));

But feel free to skip this comment, I don't expect much inconvenience with this implementation :)

Unfortunately this won't work! fileIdents have the file extension (e.g .grist or .txt or .png) appended to them by existing code - they aren't purely the hash.

It's not something we can easily change, as it's like that in all existing grist docs - so this is what we've got to do for now!

The new version is much simpler to read, thanks a lot! 🙏